Add a CryptoProvider interface and NodeCryptoProvider implementation. #1237
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcr-stripe
changed the title
dcr-stripe
force-pushed
the
dcr-crypto-interface
branch
from
8f1d3a9
to
d1283dc
Compare
| @@ -104,7 +99,8 @@ const signature = { | |||
| }); | |||
| } | |||
|
|
|||
| const expectedSignature = this._computeSignature( | |||
| cryptoProvider = cryptoProvider || getNodeCryptoProvider(); | |||
| const expectedSignature = cryptoProvider.computeHMACSignature( | |||
There was a problem hiding this comment.
This might be evil, but for async support:
- const expectedSignature = cryptoProvider.computeHMACSignature(
- ...
- );
- // Do a bunch of stuff with expectedSignature
+ const compare = (expectedSignature) => {
+ // Do a bunch of stuff with expectedSignature
+ };
+ const expectedSignature = cryptoProvider.computeHMACSignature(
+ ...
+ );
+ if (expectedSignature && typeof expectedSignature.then === 'function') {
+ return expectedSignature.then(compare);
+ } else {
+ return compare(expectedSignature);
+ }There was a problem hiding this comment.
Interesting. I think this could potentially simplify the implementation over having two separate methods... let's think about it and see how the need comes up? Either change would require updating the types though I imagine changing constructEvent's return type from Stripe.Event to Stripe.Event | Promise<Stripe.Event> wouldn't technically be backwards compatible?
Think we can merge this and handle async in a separate PR.
richardm-stripe
approved these changes
Sep 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Notify
r? @richardm-stripe
Summary
Adds an
CryptoProviderinterface and moves out the Nodecryptologic into aNodeCryptoProviderimplementation.This initially only supports computing HMAC signatures, but lets us support this in a pluggable way. The Webhooks functions are updated to support passing in a custom provider. This can be used in environments where no 'crypto' package is available (removing another Node dependency for #997).
Test plan
Added a small common set of test cases which can be re-used by any implementation to ensure they meet the contract.
Looking forwards
Some crypto APIs (eg. the Web Crypto API) only provide async functions for computing HMACs (and other primitives). In the future, we may want to extend this interface with an
asyncComputeHMACSignaturefunction that returns a promise alongsidecomputeHMACSignature. We would need to update the Webhooks code to support both sync and async computations (eg. providingasyncConstructEventandasyncVerifyHeader).